-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
net: Add buffer pool to replace connection allocation #15285
base: master
Are you sure you want to change the base?
Conversation
9e562cd
to
d243962
Compare
Our net socket connection allocations are powerful but redundant because they're implemented once in each protocol. This is not good for further optimizing and extending to other allocations, so maybe we can add a common implementation for the usage. Impact: 1. We add a `struct net_bufpool_s` as pool descriptor, which may use a little bit more memory than previous implementation (~28Bytes). 2. We share same functions between pools, so code size may shrink under some scenarios. Signed-off-by: Zhe Weng <[email protected]>
d243962
to
6a239d1
Compare
[Experimental Bot, please feedback here] No, this PR does not fully meet the NuttX requirements. While it provides a decent summary and impact assessment, it severely lacks in the testing section. Here's a breakdown of what's missing:
In short, the PR needs significant improvement in the testing and impact assessment sections before it can be properly reviewed. A more detailed summary and inclusion of issue references would also improve the PR. |
To allow dynamic allocation of these structs, performance keeps the same as the previous implementation. Signed-off-by: Zhe Weng <[email protected]>
6a239d1
to
48e57b0
Compare
****************************************************************************/ | ||
|
||
#ifndef CONFIG_NET_BLUETOOTH_MAX_CONNS | ||
# define CONFIG_NET_BLUETOOTH_MAX_CONNS 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wengzhe why to use 0 as max connections? Please for logic reasoning use 1 and later in your logic operation decrease 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because the default value of max_conns is 0 (https://github.com/apache/nuttx/blob/master/net/bluetooth/Kconfig#L57) and I found it means no limit because everywhere we use it to limit connections is checking #if max_conns > 0
. In this PR I set the sem
to INT16_MAX
in net_bufpool_init
to get similar behavior.
#if CONFIG_CAN_PREALLOC_CONNS > 0 | ||
static struct can_conn_s g_can_connections[CONFIG_CAN_PREALLOC_CONNS]; | ||
#ifndef CONFIG_CAN_MAX_CONNS | ||
# define CONFIG_CAN_MAX_CONNS 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
#if CONFIG_NET_ICMP_PREALLOC_CONNS > 0 | ||
static struct icmp_conn_s g_icmp_connections[CONFIG_NET_ICMP_PREALLOC_CONNS]; | ||
#ifndef CONFIG_NET_ICMP_MAX_CONNS | ||
# define CONFIG_NET_ICMP_MAX_CONNS 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too
static struct icmpv6_conn_s | ||
g_icmpv6_connections[CONFIG_NET_ICMPv6_PREALLOC_CONNS]; | ||
#ifndef CONFIG_NET_ICMPv6_MAX_CONNS | ||
# define CONFIG_NET_ICMPv6_MAX_CONNS 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
****************************************************************************/ | ||
|
||
#ifndef CONFIG_NET_IEEE802154_MAX_CONNS | ||
# define CONFIG_NET_IEEE802154_MAX_CONNS 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
static struct netlink_conn_s | ||
g_netlink_connections[CONFIG_NETLINK_PREALLOC_CONNS]; | ||
#ifndef CONFIG_NETLINK_MAX_CONNS | ||
# define CONFIG_NETLINK_MAX_CONNS 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
@@ -52,19 +53,19 @@ | |||
(addr1[2] == addr2[2]) && (addr1[3] == addr2[3]) && \ | |||
(addr1[4] == addr2[4]) && (addr1[5] == addr2[5])) | |||
|
|||
#ifndef CONFIG_NET_PKT_MAX_CONNS | |||
# define CONFIG_NET_PKT_MAX_CONNS 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
#if CONFIG_NET_TCP_PREALLOC_CONNS > 0 | ||
static struct tcp_conn_s g_tcp_connections[CONFIG_NET_TCP_PREALLOC_CONNS]; | ||
#ifndef CONFIG_NET_TCP_MAX_CONNS | ||
# define CONFIG_NET_TCP_MAX_CONNS 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
#if CONFIG_NET_UDP_PREALLOC_CONNS > 0 | ||
static struct udp_conn_s g_udp_connections[CONFIG_NET_UDP_PREALLOC_CONNS]; | ||
#ifndef CONFIG_NET_UDP_MAX_CONNS | ||
# define CONFIG_NET_UDP_MAX_CONNS 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
static struct usrsock_conn_s | ||
g_usrsock_connections[CONFIG_NET_USRSOCK_PREALLOC_CONNS]; | ||
#ifndef CONFIG_NET_USRSOCK_MAX_CONNS | ||
# define CONFIG_NET_USRSOCK_MAX_CONNS 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
Summary
Patches included:
Our net socket connection allocations are powerful but redundant because they're implemented once in each protocol. This is not good for further optimizing and extending to other allocations, so maybe we can add a common implementation for the usage.
Impact
struct net_bufpool_s
as pool descriptor, which may use a little bit more memory than previous implementation (~28Bytes).Testing
Mainly on SIM.